Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for borders in add_label #7147

Merged
merged 8 commits into from
Dec 19, 2019
Merged

Add support for borders in add_label #7147

merged 8 commits into from
Dec 19, 2019

Conversation

GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Dec 17, 2019 •
edited
Loading

This PR adds support for the borders parameter in the add_label() function of the _Brain object.

Closes part of #7143

@GuillaumeFavelier
Copy link
Contributor Author

The first version uses contour() and tube() for the boundary of the label so it's different compared to the reference. Don't hesitate to let me know what you think.

@codecov
Copy link

codecov bot commented Dec 17, 2019 •
edited
Loading

Codecov Report

Merging #7147 into master will increase coverage by <.01%.
The diff coverage is 85%.

@@            Coverage Diff             @@
##           master    #7147      +/-   ##
==========================================
+ Coverage   89.75%   89.75%   +<.01%     
==========================================
  Files         444      444              
  Lines       79312    79323      +11     
  Branches    12681    12684       +3     
==========================================
+ Hits        71185    71196      +11     
  Misses       5344     5344              
  Partials     2783     2783

@GuillaumeFavelier
Copy link
Contributor Author

Mayavi Pyvista borders=False Pyvista borders=True
image image image

Latest result from: https://17236-1301584-gh.circle-artifacts.com/0/dev/auto_examples/inverse/plot_time_frequency_mixed_norm_inverse.html#sphx-glr-auto-examples-inverse-plot-time-frequency-mixed-norm-inverse-py

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Dec 17, 2019 •
edited
Loading

After fixing a bug with the size of the regions:

Mayavi Pyvista
image image

Latest result from: https://17239-1301584-gh.circle-artifacts.com/0/dev/auto_examples/inverse/plot_time_frequency_mixed_norm_inverse.html#sphx-glr-auto-examples-inverse-plot-time-frequency-mixed-norm-inverse-py

@agramfort
Copy link
Member

all good https://17241-1301584-gh.circle-artifacts.com/0/dev/auto_examples/inverse/plot_time_frequency_mixed_norm_inverse.html#sphx-glr-auto-examples-inverse-plot-time-frequency-mixed-norm-inverse-py

agramfort
agramfort reviewed Dec 17, 2019
View reviewed changes
mne/viz/backends/_pysurfer_mayavi.py Outdated
@@ -128,7 +128,7 @@ def mesh(self, x, y, z, triangles, color, opacity=1.0, shading=False,

def contour(self, surface, scalars, contours, line_width=1.0, opacity=1.0,
vmin=None, vmax=None, colormap=None,
normalized_colormap=False):
normalized_colormap=False, tube=False, color=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tube seems to parametrize the size of the tube. Basically I see it can be a bool or an int. Is it different from line_width? also seeing a new param without any docstring update triggers a "why?".

@GuillaumeFavelier
Copy link
Contributor Author

@agramfort this table hopefully demonstrates the differences between lines and tubes.

By the way, I slightly modified plot_visualize_evoked.py to obtain this, let me know if that's something that could be interesting to add.

Line Tube
2019-12-18_1920x1080 2019-12-18_1920x1080

As discussed offline, I'll modify the API of contour() with a kind parameter that can be line or tube and only one width parameter to unify both.

@larsoner
Copy link
Member

I like the new version, the thicker lines are easier to see. Looking forward to seeing "line" mode with a roughly equivalent unified width. And we should consider using one of these thicker default widths in plot_evoked_field now, since it is more visible.

@GuillaumeFavelier
Copy link
Contributor Author

This is ready for reviews now @agramfort, @larsoner

And we should consider using one of these thicker default widths in plot_evoked_field now, since it is more visible.

I could totally take care of this in a follow-up PR

@larsoner
Copy link
Member

Usually the "killed" means that there is a memory problem. If you "mprof run" the example then "mprof plot" it and it's near or over 2GB then CircleCI is likely to fail in this manner.

Is it possible pyvista isn't clearing memory properly?

@GuillaumeFavelier
Copy link
Contributor Author

Where did you see 'killed'? Do you mean #7150?

@GuillaumeFavelier
Copy link
Contributor Author

It's totally possible that it's related to memory management, I will investigate

@larsoner
Copy link
Member

Yes sorry in #7150

@agramfort agramfort merged commit 6c73272 into mne-tools:master Dec 19, 2019
@agramfort
Copy link
Member

thx @GuillaumeFavelier ! one more down !

@GuillaumeFavelier GuillaumeFavelier deleted the brain_add_label_borders branch January 8, 2020 15:12
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
@GuillaumeFavelier @AdoNunes
Add support for borders in add_label (mne-tools#7147)
a1b29e2 
* Try support for tube as borders

* Trigger plot_time_frequency_mixed_norm_inverse.py

* Fix scalar_thresh

* Improve coverage

* Fix plot_visualize_evoked.py

* Update contour API

* Fix style
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
@GuillaumeFavelier @AdoNunes
Add support for borders in add_label (mne-tools#7147)
11fa4d8 
* Try support for tube as borders

* Trigger plot_time_frequency_mixed_norm_inverse.py

* Fix scalar_thresh

* Improve coverage

* Fix plot_visualize_evoked.py

* Update contour API

* Fix style
GuillaumeFavelier referenced this pull request in GuillaumeFavelier/mne-python Aug 24, 2020
@GuillaumeFavelier
Use https://github.com/mne-tools/mne-python/pull/7147\#issuecomment-5…
671abd8 
…66963090
larsoner referenced this pull request Sep 8, 2020
@larsoner @GuillaumeFavelier
MRG: Recreate our helmet graphic (#8116)
13a5ea4 
* WIP: Recreate our helmet

* Fix roll

* Use https://github.com/mne-tools/mne-python/pull/7147\#issuecomment-566963090

* Change width slightly

* Add polygon_offset parameter to surface()

* Add polygon_offset parameter to mesh()

* Use polygon_offset to resolve coincident topology

* Fix contour()

* Use lines instead of tubes

* Fix pickable issue

* Fix cropped helmet with pyvista

Co-authored-by: Guillaume Favelier <[email protected]>
marsipu referenced this pull request in marsipu/mne-python Oct 14, 2020
@larsoner @GuillaumeFavelier @marsipu
MRG: Recreate our helmet graphic (mne-tools#8116)
46baa79 
* WIP: Recreate our helmet

* Fix roll

* Use https://github.com/mne-tools/mne-python/pull/7147\#issuecomment-566963090

* Change width slightly

* Add polygon_offset parameter to surface()

* Add polygon_offset parameter to mesh()

* Use polygon_offset to resolve coincident topology

* Fix contour()

* Use lines instead of tubes

* Fix pickable issue

* Fix cropped helmet with pyvista

Co-authored-by: Guillaume Favelier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agramfort agramfort agramfort left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GuillaumeFavelier @agramfort @larsoner